feat: unifying npm registry requests with caching #641
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
…ests # Conflicts: # app/composables/useNpmRegistry.ts # app/utils/package-name.ts
This reverts commit fe1fd2d.
OrbisK
changed the title
|
this looks good! would you investigate why the browser tests might be failing? |
Working on it. Maybe you know what the issue is. e.g. the org page is broken. https://npmxdev-git-fork-orbisk-feat-concept-npm-requests-poetry.vercel.app/@nuxt 
|
|
I think the problem is that I think I have a fix. |
|
I am super confused. Are |
|
Okay. The request returns the data and 200 but cors is still blocking it 🤔 |
|
$npmRegistry should be idempotent, yes. (it should be safe to call in either case) if there's stale data (or missing data) from the server, then it will refetch on the client |
|
Progress: I have fixed it locally. But i am not sure how... |
looks good. 👍🏽 Not sure how to fix the Nuxt instance issue though. I think this might be a Nuxt bug. |
|
@danielroe is attempting to deploy a commit to the serhalp's projects Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/composables/npm/useNpmSearch.ts (1)
53-215: Consider splittinguseNpmSearchinto smaller helpers.It now covers initial search, single-character lookup, paging, caching, and watchers; breaking it up would improve readability and future changes. As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
| batch.map(async name => { | ||
| const encoded = encodePackageName(name) | ||
| const data = await $fetch<{ downloads: number }>( | ||
| `${NPM_API}/downloads/point/last-week/${encoded}`, | ||
| const { data } = await $npmApi<{ downloads: number }>( | ||
| `/downloads/point/last-week/${encoded}`, | ||
| ) | ||
| return { name, downloads: data.downloads } | ||
| }), |
There was a problem hiding this comment.
Missing options parameter for scoped package downloads.
The scoped package individual fetches do not receive the options parameter (which includes signal for request cancellation). This is inconsistent with the unscoped bulk fetch on line 34, which correctly passes options. If the parent request is aborted, these requests will continue executing.
🔧 Proposed fix to pass options
batch.map(async name => {
const encoded = encodePackageName(name)
const { data } = await $npmApi<{ downloads: number }>(
`/downloads/point/last-week/${encoded}`,
+ options,
)
return { name, downloads: data.downloads }
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| batch.map(async name => { | |
| const encoded = encodePackageName(name) | |
| const data = await $fetch<{ downloads: number }>( | |
| `${NPM_API}/downloads/point/last-week/${encoded}`, | |
| const { data } = await $npmApi<{ downloads: number }>( | |
| `/downloads/point/last-week/${encoded}`, | |
| ) | |
| return { name, downloads: data.downloads } | |
| }), | |
| batch.map(async name => { | |
| const encoded = encodePackageName(name) | |
| const { data } = await $npmApi<{ downloads: number }>( | |
| `/downloads/point/last-week/${encoded}`, | |
| options, | |
| ) | |
| return { name, downloads: data.downloads } | |
| }), |
4 participants
I think it is not an easy change to refactor all npm requests to
useNpmRegistry(yet). As a first step, we could migrate all npm requests to$npmRegistryand$npmApi. Wdyt?